Conversation
Document the MI300X aggregate and disaggregated validation plan, cluster assumptions, safety boundaries, and current srt-slurm development pin.
Add the reusable MI300X cluster runtime profile and a minimal stable-vLLM fixed-sequence aggregate recipe for functional bring-up.
eb12f0e to
cfd30df
Compare
|
Closing because the project scope is now limited to porting active, nondeprecated AMD multi-node configurations to srt-slurm. This PR is outside that scope. |
|
Reopening: this PR predates the recent out-of-scope configuration-porting detour and was closed by mistake during an overly broad cleanup. Its prior state is being restored. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the un-cleaned REMOTE_RUNTIME directories in runners/launch_mi300x-amds-srt.sh could cause cross-run collisions on the shared /raid path — RUN_KEY makes each directory uniquely named per run/attempt/runner, so this is purely an accumulating disk-usage leak (already flagged inline), not a race or corruption risk between concurrent jobs.
Extended reasoning...
This run reported three confirmed findings (doc/commit mismatch in AMD_BRINGUP.md, unbounded disk growth from un-cleaned REMOTE_RUNTIME/REMOTE_RESULTS staging directories, and a missing pipefail in the result-retrieval tar|base64 pipeline) which will be posted as inline comments, so a full approve/defer-with-summary review is not appropriate here. I additionally re-read runners/launch_mi300x-amds-srt.sh and launch_mi355x-amds-srt.sh to check on my own prior inline comments from the earlier review pass: the missing-flock issue on ensure_git_checkout has been fixed in both files (both now take a per-target flock before cloning/checking out), but the un-cleaned .incomplete.* quarantine directories and the unconditional (ungated) Infera/ATOM checkouts for image types that don't need them remain unaddressed — per instructions I'm not restating those since they're unchanged prior findings, not new information. The one net-new item worth surfacing is the ruled-out race-condition question on REMOTE_RUNTIME, which is concrete and distinct from the confirmed leak finding.
|
|
||
| - repository: `SemiAnalysisAI/srt-slurm` | ||
| - branch: `agent/amd-multinode-runtime` | ||
| - commit: `315e4b06a7e0806194a646ea21832e750e896a46` |
There was a problem hiding this comment.
🟡 New doc states the pinned srt-slurm commit is 315e4b06a7e0806194a646ea21832e750e896a46, but the launcher scripts added in the same PR (runners/launch_mi300x-amds-srt.sh:13, runners/launch_mi355x-amds-srt.sh:14) actually pin 5ecfb13d1ba0960045482f1ef006312d8729d37a.
Extended reasoning...
An engineer or on-call debugging a failed srt-slurm run reads AMD_BRINGUP.md to find the pinned commit, checks out 315e4b06... to reproduce, and investigates the wrong tree/behavior since the launchers actually run 5ecfb13d... — wasting debugging time and potentially reaching wrong conclusions about what code is under validation.
Verification: nit. Documentation/code inconsistency, all introduced in this PR. AMD_BRINGUP.md:12 states "Current development pin for both AMD launchers ... commit: 315e4b06a7e0806194a646ea21832e750e896a46", but both launchers pin a different SHA: runners/launch_mi300x-amds-srt.sh:7 SRT_SLURM_COMMIT="5ecfb13d1ba0960045482f1ef006312d8729d37a" and runners/launch_mi355x-amds-srt.sh:8 same value. The…
| REMOTE_RUNTIME="${REMOTE_BASE}/runtime/inferencex-${RUN_KEY}" | ||
| REMOTE_SRT_RUNTIME="${REMOTE_BASE}/runtime/srt-slurm-${SRT_SLURM_COMMIT}" | ||
| REMOTE_INFERA_RUNTIME="${REMOTE_BASE}/runtime/infera-${INFERA_COMMIT}" | ||
| REMOTE_ATOM_RUNTIME="${REMOTE_BASE}/runtime/atom-${ATOM_COMMIT}" | ||
| REMOTE_RESULTS="${REMOTE_BASE}/results" | ||
| WORK_DIR="${GITHUB_WORKSPACE}/.srt-slurm-${RUN_KEY}" | ||
| SRT_REPO_DIR="${WORK_DIR}/srt-slurm" | ||
| mkdir -p "$WORK_DIR" | ||
|
|
||
| # The login and compute nodes do not share a filesystem. Stage only the | ||
| # unchanged InferenceX benchmark client and immutable public container images | ||
| # onto every eligible node. The cluster has nine nodes and this validation | ||
| # excludes two, so the staging allocation must cover all seven remaining nodes. | ||
| # The batch job exits normally; it does not cancel or preempt any allocation. | ||
| RUNTIME_ARCHIVE="${WORK_DIR}/inferencex-benchmark.tar.gz" | ||
| tar -C "$GITHUB_WORKSPACE" -czf "$RUNTIME_ARCHIVE" utils/bench_serving | ||
| RUNTIME_PAYLOAD=$(base64 -w0 "$RUNTIME_ARCHIVE") | ||
| STAGE_SCRIPT="${WORK_DIR}/stage-runtime.sbatch" | ||
| cat > "$STAGE_SCRIPT" <<EOF | ||
| #!/usr/bin/env bash | ||
| #SBATCH --partition=${SLURM_PARTITION} | ||
| #SBATCH --nodes=7 | ||
| #SBATCH --ntasks-per-node=1 | ||
| #SBATCH --cpus-per-task=1 | ||
| #SBATCH --time=00:45:00 | ||
| #SBATCH --exclude=${EXCLUDED_NODES} | ||
| #SBATCH --job-name=${RUNNER_NAME:-mi300x-srt}-stage | ||
| set -euo pipefail | ||
| source_archive="/tmp/inferencex-benchmark-source-\${SLURM_JOB_ID}.tar.gz" | ||
| node_archive="/tmp/inferencex-benchmark-\${SLURM_JOB_ID}.tar.gz" | ||
| printf '%s' '${RUNTIME_PAYLOAD}' | base64 -d > "\$source_archive" | ||
| sbcast --force "\$source_archive" "\$node_archive" | ||
| srun --ntasks-per-node=1 bash -c ' | ||
| set -euo pipefail | ||
| runtime="${REMOTE_RUNTIME}" | ||
| srt_runtime="${REMOTE_SRT_RUNTIME}" | ||
| infera_runtime="${REMOTE_INFERA_RUNTIME}" | ||
| atom_runtime="${REMOTE_ATOM_RUNTIME}" | ||
| export ENROOT_RUNTIME_PATH="\${TMPDIR:-/tmp}/enroot-runtime-\${UID}" | ||
| mkdir -p "\$ENROOT_RUNTIME_PATH" "\$runtime" "${REMOTE_RESULTS}" "${REMOTE_BASE}/containers" |
There was a problem hiding this comment.
🟡 New srt-slurm launchers stage a fresh per-run directory tree on shared/persistent storage (REMOTE_RUNTIME="$REMOTE_BASE/runtime/inferencex-$RUN_KEY" on /raid, plus REMOTE_RESULTS/$JOB_ID; mi355x equivalent is SHARED_RESULTS/$JOB_ID on /it-share) and never remove it after the run finishes, unlike the pre-existing quarantine-dir issue this only triggers on error paths -- this happens on every single successful run.
Extended reasoning...
Every GHA sweep invocation of qwen3-0.6b-fp16-mi300x-vllm-srt-agg/-disagg or the atom-infera/sglang srt lanes mkdir's a uniquely-keyed runtime dir (line 53/92) and extracts the benchmark client tarball into it, and srtctl writes per-job results/logs under REMOTE_RESULTS/$JOB_ID (line 57, retrieved at line 248) -- none of this is ever rm -rf'd by the launcher (grep for rm -rf against REMOTE_RUNTIME/REMOTE_RESULTS/SHARED_RESULTS finds nothing), so /raid and /it-share accumulate one new directory per CI run forever, exhausting shared disk/inode capacity that other jobs on the cluster depend on. A correct fix adds cleanup (rm -rf on success, or a trap) for the per-run runtime and results directories in both launch_mi300x-amds-srt.sh and launch_mi355x-amds-srt.sh.
Verification: nit — real but slow, small-per-run resource leak; nothing breaks immediately. runners/launch_mi300x-amds-srt.sh:52-53 build a per-run key RUN_KEY="${GITHUB_RUN_ID:-manual}-${GITHUB_RUN_ATTEMPT:-0}-${RUNNER_NAME:-runner}" and REMOTE_RUNTIME="${REMOTE_BASE}/runtime/inferencex-${RUN_KEY}" (REMOTE_BASE=/raid/hf-hub-cache/inferencex/srt-slurm, line 15). Since GITHUB_RUN_ID is unique per…
| RESULT_PAYLOAD=$(srun --partition="$SLURM_PARTITION" --nodes=1 --ntasks=1 \ | ||
| --cpus-per-task=1 --time=00:05:00 --nodelist="$JOB_BATCH_HOST" \ | ||
| bash -c "tar -C '${REMOTE_RESULTS}/${JOB_ID}' -czf - . | base64 -w0") |
There was a problem hiding this comment.
🟡 The result-retrieval srun ... bash -c "tar ... | base64 -w0" (lines 246-248) runs without set -o pipefail (or even set -e) inside the remote bash -c, unlike every other embedded script block in this diff -- so if tar fails (e.g. ${REMOTE_RESULTS}/${JOB_ID} is missing because the srt-slurm job errored before writing results), the pipeline's exit status is masked by the always-succeeding base64, and RESULT_PAYLOAD silently becomes empty/truncated.
Extended reasoning...
When the srt-slurm job fails before creating its results directory, tar -C '<missing-dir>' -czf - errors to stderr but the piped base64 -w0 still exits 0, so the outer $(...) capture succeeds with an empty payload; the very next line, printf '%s' "$RESULT_PAYLOAD" | base64 -d | tar -xzf - -C "$RETRIEVE_DIR", then aborts the whole launcher (via the top-level set -euo pipefail) with a generic 'not in gzip format' tar error at line 249, before the script ever reaches its own JOB_STATE/JOB_EXIT diagnostic at line 283 -- so on-call engineers see a confusing tar-decompression failure instead of the real cause of the srt-slurm job failure, and no useful srt-slurm job-state diagnostic is ever printed for that run.
Verification: Severity: nit. The missing set -o pipefail inside the remote bash -c is real: line 248 runs bash -c "tar -C '${REMOTE_RESULTS}/${JOB_ID}' -czf - . | base64 -w0" with no pipefail, unlike the staging block which sets set -euo pipefail inside its own srun ... bash -c (lines 85-86). So a failing tar (missing results dir when the srt-slurm job errored) has its exit status masked by…
c84fa81 to
c94d0a4
Compare
0d7ca6f to
6d439d6
Compare
e97493d to
97a721a
Compare
|
Closed due to inactivity for 30+ days. |
1 participant
Summary
This stacked validation PR exercises the combined AMD platform work plus ATOM backend and Infera routing support from SemiAnalysisAI/srt-slurm#3 on MI300X and MI355X.
It intentionally uses Qwen3-0.6B and short fixed-sequence workloads to validate orchestration, routing, KV transfer, cleanup, and artifact collection rather than tune performance.
Dependency structure
Current runtime pins:
5ecfb13d1ba0960045482f1ef006312d8729d37a8ed8f1728c745d4e91ba9eaa09ed81159aa57e412ab42bc2c64d1ad04f698c396da48473e71a6dbbConfigurations
For both MI300X and MI355X:
The launcher mounts the pinned Infera and ATOM sources over the stable
rocm/infera:atom-v0.1.1image so validation is reproducible without rebuilding the container.Implementation and local validation
bash -n.git diff --checkpasses.EVALSassignment and has a regression test ensuring every emitted matrix variable is defined. This fixes the previously red wrapper where all materialized workload jobs were green.Hardware evidence
MI300X
Run 31524554022, exact tested head
f9bdbe1c583182be2071c99fe0bb22cdcb68efb7:COMPLETED 0:0; two workers registered; c1/c4 succeeded; 25 HTTP 200 responses; no fatal server markers.COMPLETED 0:0; 25/25 producer writes, 25/25 decode first-token injections, two TCP transports, zero RDMA installs, and zero registration/transfer/socket failures.MI355X
COMPLETED 0:0; 25/25 writes, 25/25 decode injections, two TCP transports, zero RDMA installs, zero transfer failures; c1/c4 results and logs uploaded.COMPLETED 0:0; two workers registered; c1/c4 succeeded; artifacts uploaded.The prior MI355X attempt that returned HTTP 200 but failed every KV write is not counted as success. Its root cause was Mooncake selecting RDMA after HCA discovery despite
protocol=tcp. Exact corrected runs injectMC_FORCE_TCP=true; their uploaded logs prove TCP installation and clean KV transfer.Current head
PR head is
eb814bbf0894e17132d43b241e4671388e698ba9. The hardware-tested runtime content is unchanged fromf9bdbe1; the final commit only repairs the E2E matrix output and adds its regression test. Final-head workflow proof: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/31526765208 completed SUCCESS, including matrix generation, the MI300X aggregate workload, result collection, and success-rate calculation.Note
Medium Risk
Large CI/benchmark surface area with new Slurm staging and multinode orchestration paths; no auth changes, but misconfigured matrix or launcher logic could break E2E sweeps or shared cluster jobs.
Overview
Adds srt-slurm–driven correctness lanes on MI300X and MI355X for the ATOM backend with Infera KV-aware routing: two-worker aggregate recipes and 1P/1D disagg with Mooncake TCP KV transfer, wired through new matrix keys in
amd-master.yamland custombenchmark_serving.pyhooks in the recipes.MI300X/MI355X launchers gain dedicated
*-amds-srt.shpaths (selected whenCONFIG_FILEis set) that pin srt-slurm, stage squashfs/runtime on Slurm, mount pinned Infera/ATOM git checkouts overrocm/infera:atom-v0.1.1, runsrtctl apply, and collect fixed-sequence JSON artifacts. Cluster profiles include the Infera/ATOM container image.The same diff also lands the broader AMD srt-slurm recipe set (vLLM/MoRI-IO on MI300X, SGLang/MoRI on MI355X including Qwen3.5 TP8 disagg),
AMD_BRINGUP.md, and perf-changelog entries. E2E workflow restores the missingEVALSmatrix assignment and adds a test that every emitted matrix variable is defined.Reviewed by Cursor Bugbot for commit 4a98aa9. Bugbot is set up for automated code reviews on this repo. Configure here.